Skip to content

Conversation

@hemanthboyina
Copy link

compute_table_stats fails with CommitFailedException when concurrent writes modify table metadata during statistics computation.

@github-actions github-actions bot added the core label Jan 26, 2026
Copy link
Contributor

@dramaticlly dramaticlly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shall add commit retry like other PendingUpdate implementation class, RemoveSnapshots is a good example. But we can probably simplify the unit test a bit


// Skip if no changes
if (base == newMetadata) {
LOG.info("No statistics changes to commit, skipping");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think debug shall be sufficient here or we can remove it entirely.

Copy link
Contributor

@dramaticlly dramaticlly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

snapshotId, "/some/statistics/file.puffin", 100, 42, ImmutableList.of());

TestTables.TestTableOperations ops = table.ops();
ops.failCommits(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, I think $maxAttempt+1 would be helpful here like

    ops.failCommits(TableProperties.COMMIT_NUM_RETRIES_DEFAULT + 1);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use
Integer.parseInt(
table.properties().getOrDefault(TableProperties.COMMIT_NUM_RETRIES, "4") + 1 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants